Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue-1864: Fix fill misbehaving when drawing was partly outside border #1865

Merged
merged 6 commits into from
Aug 18, 2024

Conversation

MrStevns
Copy link
Member

Fixes #1864

@MrStevns
Copy link
Member Author

In addition to fixing the bug not being filled correct, it will also now take the border into account properly, as talked about here. https://discord.com/channels/342369662710972417/342670204351938562/1265361324456804362

fill-behavior-fix2

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small remark, looks good to me otherwise. Thanks for fixing!

core_lib/src/graphics/bitmap/bitmapimage.cpp Outdated Show resolved Hide resolved
@J5lx J5lx self-assigned this Aug 11, 2024
As this is now handled by the fill algorithm, no need for additional checks.
We can't both check whether we're outside and then inside :3
Assuming we should make it seem like the canvas is infinite, it's more appropriate to move the fill point inside the bounds, when we're filling outside the bounds.
When the point is outside the fill bounds
@MrStevns
Copy link
Member Author

Thanks for reviewing Jacob!

I've addressed your comment and made some small adjustments in order make the bucket tool always fills within the bounds. The logic for that was already there, it was just being ignored because we had a contradictory check, so I simplified it a bit and now that works too.

In practice this just means that when you try to fill outside the bounds of the image, it will move the fill point inside, rather than ignore the fill command.

Copy link
Member

@J5lx J5lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@J5lx J5lx merged commit ac2054e into pencil2d:master Aug 18, 2024
10 checks passed
@MrStevns MrStevns modified the milestones: 0.7.1, 0.8.0 Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Fill misbehaving in some cases
2 participants